Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New QL LED driver #3

Open
wants to merge 20 commits into
base: eos-s3-support
Choose a base branch
from

Conversation

spingaliQL
Copy link

I have added a new led driver under drivers/led. It uses FPGA IP, which as of now doesn't give any option to select LED color, blink duration, etc.
FPGA bit stream is stored in a header file. The driver itself loads the FPGA, instead of manually loading outside through Jlink. The driver also does the IO mux setting and FBIO selection.
I have added a sample qf_helloworldhw under samples, which enable config for this driver.
I have added a new function in soc.c to select FBIOs.
In soc.c:eos_s3_init() the function program_fpga_ip() is called based on a macro enable. If any driver wants to wants FPGA to be programmed it will define this function and enable this macro.
NOTE: as I'm merging all changes from Quicklogic-corp repo, the manifest file is also getting merged, which is modified to take hal_quicklogic repo from Quicklogic-corp. I'll undo this change on antmicro/zephyr after merging all these changes.

spingaliQL and others added 4 commits May 27, 2020 18:01
@spingaliQL
Copy link
Author

I have further made changes. I have moved "qf_helloworldhw" from samples folder to samples/drivers folder and renamed as "led_eos_s3_1".
I have made changes to load litex PWM FPGA from M4 code. This driver is used in rgb_led and fade_led samples.

@@ -11,7 +11,7 @@ config BOARD
if PWM

config PWM_LITEX
select SOC_EOS_S3_FPGA
select EOS_S3_PROGRAM_FPGA

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not change this selection, because SOC_EOS_S3_FPGA configuration tells software if we want to initialize FPGA or not here. Without it clocks won't be set up.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main change I made in these commits is to program FPGA from M4 program itself, instead of doing it outside using Jlink. To enable this feature I have added EOS_S3_PROGRAM_FPGA. If someone wants to use the old way, one can enable SOC_EOS_S3_FPGA. I have not made any changes in this flow. Since all clocks are enabled with EOS_S3_PROGRAM_FPGA, other is not required. So EOS_S3_PROGRAM_FPGA and SOC_EOS_S3_FPGA are mutually exclusive.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does eos_s3_pwm_ip.h contain PWM_LITEX? If not you must add separate config entry like explained here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it doesn't.
I'll add separate config entry.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added new configuration. However I understand from our H/W team that we are using litex pwm IP only, the bit stream is generated using symbi flow tools (with EOS S3 support). Our driver just loads the FPGA IP. It doesn't have full functions. For that I have to use pwm litex driver. So I have included that too.

@@ -4,5 +4,6 @@ zephyr_sources_ifdef(CONFIG_HT16K33 ht16k33.c)
zephyr_sources_ifdef(CONFIG_LP3943 lp3943.c)
zephyr_sources_ifdef(CONFIG_LP5562 lp5562.c)
zephyr_sources_ifdef(CONFIG_PCA9633 pca9633.c)
zephyr_sources_ifdef(CONFIG_EOS_S3_LED1 led_eos_s3_1.c)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just name it led_eos_s3.c ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are planning to have one more version of LED controller. I'll call this led_eos_s3_basic.c. "_1" is not giving good meaning. I'll apply this change in all related files/config.

@@ -24,5 +24,6 @@ source "drivers/led/Kconfig.ht16k33"
source "drivers/led/Kconfig.lp3943"
source "drivers/led/Kconfig.lp5562"
source "drivers/led/Kconfig.pca9633"
source "drivers/led/Kconfig.eos_s3_1"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just name it Kconfig.eos_s3 ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll rename to Kconfig.eos_s3_basic

drivers/led/Kconfig.eos_s3_1 Outdated Show resolved Hide resolved
# Copyright (c) 2018 Workaround GmbH
# SPDX-License-Identifier: Apache-2.0

config EOS_S3_LED1

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without 1? I'm not sure why you use this nomenclature with 1 at the end. Will it be more versions of driver?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a simple LED controller and an advanced one. I'll call all "1" as "basic"

soc/arm/quicklogic_eos_s3/soc.c Show resolved Hide resolved
soc/arm/quicklogic_eos_s3/soc.h Outdated Show resolved Hide resolved
drivers/led/led_eos_s3_1.c Outdated Show resolved Hide resolved
drivers/pwm/pwm_eos_s3.c Outdated Show resolved Hide resolved
soc/arm/quicklogic_eos_s3/fpga_loader.h Outdated Show resolved Hide resolved
@kowalewskijan
Copy link

@spingaliQL If you want to check your code formatting to fit Zephyr standards, you can use a script provided in Zephyr repository. It is stored in: zephyr/scripts/checkpatch.pl. To use it you just simply: ./checkpatch.pl -f path/to/file

mateusz-holenko pushed a commit that referenced this pull request Jun 23, 2020
This makes the gatt metrics also available for
gatt write-without-rsp-cb so it now prints the rate of each write:

uart:~$ gatt write-without-response-cb 1e ff 10 10
Write #1: 16 bytes (0 bps)
Write #2: 32 bytes (3445948416 bps)
Write #3: 48 bytes (2596929536 bps)
Write #4: 64 bytes (6400 bps)
Write #5: 80 bytes (8533 bps)
Write zephyrproject-rtos#6: 96 bytes (10666 bps)
Write zephyrproject-rtos#7: 112 bytes (8533 bps)
Write zephyrproject-rtos#8: 128 bytes (9955 bps)
Write zephyrproject-rtos#9: 144 bytes (11377 bps)
Write zephyrproject-rtos#10: 160 bytes (7680 bps)
Write zephyrproject-rtos#11: 176 bytes (8533 bps)
Write zephyrproject-rtos#12: 192 bytes (9386 bps)
Write Complete (err 0)
Write zephyrproject-rtos#13: 208 bytes (8533 bps)
Write zephyrproject-rtos#14: 224 bytes (9244 bps)
Write zephyrproject-rtos#15: 240 bytes (9955 bps)
Write zephyrproject-rtos#16: 256 bytes (8000 bps)

Signed-off-by: Luiz Augusto von Dentz <[email protected]>
@spingaliQL
Copy link
Author

@kowalewskijan Thank you very much for detailed review and suggesting the script for code formatting. It is very helpful. I have started working on the changes recommended. Please see my replies and let me know if you have further comments. Once I complete the changes, I'll inform you in same PR.

spingaliQL and others added 7 commits June 25, 2020 17:28
-Change QF dts file with new partitions
-Removed sleep which was added for testing
-Reinitialize C02 clock divider so that it will not change after running
boot loader
-Added new config EOS_S3_PWM which loads litex pwm ip bit stream and then uses litex pwm driver
-Renamed EOS_S3_LED1 as EOS_S3_LED_BASIC; in configuration files, driver files and sample files
-Cleanup in fpga_loader.c file. Instead of using hardcoded register addresses using HAL defined macros
-Moved the common code used in soc.c and fpga_loader.c to a common function enable_fpga_clocks()
-Resolved coding style related issues in all .c files
Copy link
Author

@spingaliQL spingaliQL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have made below changes as per the review:
-Added new config EOS_S3_PWM which loads litex pwm ip bit stream and then uses litex pwm driver.
-Renamed EOS_S3_LED1 as EOS_S3_LED_BASIC; in configuration files, driver files and sample files
-Cleanup in fpga_loader.c file. Instead of using hardcoded register addresses using HAL defined macros
-Moved the common code used in soc.c and fpga_loader.c to a common function enable_fpga_clocks()
-Resolved coding style related issues in all .c files

@@ -11,7 +11,7 @@ config BOARD
if PWM

config PWM_LITEX
select SOC_EOS_S3_FPGA
select EOS_S3_PROGRAM_FPGA
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added new configuration. However I understand from our H/W team that we are using litex pwm IP only, the bit stream is generated using symbi flow tools (with EOS S3 support). Our driver just loads the FPGA IP. It doesn't have full functions. For that I have to use pwm litex driver. So I have included that too.

@spingaliQL
Copy link
Author

Hi @kowalewskijan
I have resolved almost all review comments, except these:
-Moving the IP bitstream header files to separate repo: Our H/W team is working on the repo for keeping all our IPs. Once it is ready, I'll refer the header files from there and remove from drivers folder.
-I tried to use k_sleep in fpga_loader.c as you suggested. It doesn't work, system hangs. I don't know if k_sleep can be used here as we are still executing system init function. Please suggest how to use sleep function.
Thank you.

#if 0
k_sleep(100);
#else
for (i = 0; i < 60; i++)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we wait for reset pulse as stated above:

/* wait some time for fpga to get reset pulse */

Maybe there is a kind of flag that could provide an information about it? If yes, then we could have just something like this:

Suggested change
for (i = 0; i < 60; i++)
while (!REG->FLAG) {
PMU->GEN_PURPOSE_1 = i << 4;
k_sleep(1);
}

/* Set C02 clock to default value,
* if any prev app such as boot loader changes it will be set back
*/
CRU->CLK_CTRL_B_0 = 0x204;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any macros in HAL that we can use instead of a magic value?


}

void program_fpga_ip(void)
Copy link

@kowalewskijan kowalewskijan Jul 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about loading bitstream inside other drivers and I came to conclusion that the best solution would be to introduce a new type of driver in Zephyr - FPGA manager. We should add drivers/fpga directory in Zephyr, add global Kconfig and Kconfig.eos_s3 and add there a new driver exclusively for FPGA control, like: FPGA init, FPGA bitstream loading etc. So in the end there would be 2 separate layers - 1st: FPGA control related and 2nd: driver for IPcore. This would allow us to upload bitstream at early boot stage and later use IP core drivers without special FPGA managing functions.

@@ -21,7 +21,7 @@
#error Unsupported flash driver
#endif

#define FLASH_TEST_REGION_OFFSET 0xff000
#define FLASH_TEST_REGION_OFFSET 0x100000

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please undo if this is a leftover

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have M4 application image starting from offset 0x0008’0000 to 0x000F’FFFF. So we can not use the location 0xff000 for this test. Since the offset is hard-coded within the application I have modified it for testing. Shall I change it to DT_FLASH_AREA_STORAGE_OFFSET as in nvs sample? If not, I don't know what is the way out.

Note: I have given image-3 size wrong as 0x20000 in quick_feather.dts. I'll fix it in next commit.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can fix it like here

u32_t chunk_cnt = 0;
volatile u32_t *gFPGAPtr = (volatile u32_t *)image_ptr;

IO_MUX->PAD_19_CTRL = 0x00000180;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any macros to use instead of a magic value?

k_sleep(100);
#else
for (i = 0; i < 60; i++)
PMU->GEN_PURPOSE_1 = i << 4;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are multiple of these for loops, please add a static function dedicated to wait process


CRU->FB_SW_RESET = 0;
CRU->FB_MISC_SW_RST_CTL = 0;
PMU->GEN_PURPOSE_1 = 0x90;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any macros to use instead of a magic value?

for (i = 0; i < 60; i++)
PMU->GEN_PURPOSE_1 = i << 4;
#endif
IO_MUX->PAD_19_CTRL = 0x000009a0;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any macros to use instead of a magic value?

@@ -143,6 +173,7 @@ static void eos_s3_fpga_init(void)

CRU->C08_X4_CLK_GATE = C08_X4_CLK_GATE_PATH_0_ON;


Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove redundant line

spingaliQL and others added 2 commits July 7, 2020 17:15
Kept the original offset as it is. For quick feather board offset is
changed to 0x100000
Corrected the image-3 size in quick feather dts file
@github-actions
Copy link

github-actions bot commented Oct 6, 2020

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Oct 6, 2020
mateusz-holenko pushed a commit that referenced this pull request Dec 5, 2020
The _ldiv5() is an optimized divide-by-5 function that is smaller and
faster than the generic libgcc implementation.

Yet it can be made even smaller and faster with this replacement
implementation based on a reciprocal multiplication plus some tricks.

For example, here's the assembly from the original code on ARM:

_ldiv5:
        ldr     r3, [r0]
        movw    ip, zephyrproject-rtos#52429
        ldr     r1, [r0, #4]
        movt    ip, 52428
        adds    r3, r3, #2
        push    {r4, r5, r6, r7, lr}
        mov     lr, #0
        adc     r1, r1, lr
        adds    r2, lr, lr
        umull   r7, r6, ip, r1
        lsr     r6, r6, #2
        adc     r7, r6, r6
        adds    r2, r2, r2
        adc     r7, r7, r7
        adds    r2, r2, lr
        adc     r7, r7, r6
        subs    r3, r3, r2
        sbc     r7, r1, r7
        lsr     r2, r3, #3
        orr     r2, r2, r7, lsl zephyrproject-rtos#29
        umull   r2, r1, ip, r2
        lsr     r2, r1, #2
        lsr     r7, r1, zephyrproject-rtos#31
        lsl     r1, r2, #3
        adds    r4, lr, r1
        adc     r5, r6, r7
        adds    r2, r1, r1
        adds    r2, r2, r2
        adds    r2, r2, r1
        subs    r2, r3, r2
        umull   r3, r2, ip, r2
        lsr     r2, r2, #2
        adds    r4, r4, r2
        adc     r5, r5, #0
        strd    r4, [r0]
        pop     {r4, r5, r6, r7, pc}

And here's the resulting assembly with this commit applied:

_ldiv5:
        push    {r4, r5, r6, r7}
        movw    r4, zephyrproject-rtos#13107
        ldr     r6, [r0]
        movt    r4, 13107
        ldr     r1, [r0, #4]
        mov     r3, #0
        umull   r6, r7, r6, r4
        add     r2, r4, r4, lsl #1
        umull   r4, r5, r1, r4
        adds    r1, r6, r2
        adc     r2, r7, r2
        adds    ip, r6, r4
        adc     r1, r7, r5
        adds    r2, ip, r2
        adc     r2, r1, r3
        adds    r2, r4, r2
        adc     r3, r5, r3
        strd    r2, [r0]
        pop     {r4, r5, r6, r7}
        bx      lr

So we're down to 20 instructions from 36 initially, with only 2 umull
instructions instead of 3, and slightly smaller stack footprint.

Signed-off-by: Nicolas Pitre <[email protected]>
fzdobylak pushed a commit that referenced this pull request Dec 8, 2022
This patch reworks how fragments are handled in the net_buf
infrastructure.

In particular, it removes the union around the node and frags members in
the main net_buf structure. This is done so that both can be used at the
same time, at a cost of 4 bytes per net_buf instance.
This implies that the layout of net_buf instances changes whenever being
inserted into a queue (fifo or lifo) or a linked list (slist).

Until now, this is what happened when enqueueing a net_buf with frags in
a queue or linked list:

1.1 Before enqueueing:

 +--------+      +--------+      +--------+
 |#1  node|\     |#2  node|\     |#3  node|\
 |        | \    |        | \    |        | \
 | frags  |------| frags  |------| frags  |------NULL
 +--------+      +--------+      +--------+

net_buf #1 has 2 fragments, net_bufs #2 and #3. Both the node and frags
pointers (they are the same, since they are unioned) point to the next
fragment.

1.2 After enqueueing:

 +--------+      +--------+      +--------+      +--------+      +--------+
 |q/slist |------|#1  node|------|#2  node|------|#3  node|------|q/slist |
 |node    |      | *flag  | /    | *flag  | /    |        | /    |node    |
 |        |      | frags  |/     | frags  |/     | frags  |/     |        |
 +--------+      +--------+      +--------+      +--------+      +--------+

When enqueing a net_buf (in this case #1) that contains fragments, the
current net_buf implementation actually enqueues all the fragments (in
this case #2 and #3) as actual queue/slist items, since node and frags
are one and the same in memory. This makes the enqueuing operation
expensive and it makes it impossible to atomically dequeue. The `*flag`
notation here means that the `flags` member has been set to
`NET_BUF_FRAGS` in order to be able to reconstruct the frags pointers
when dequeuing.

After this patch, the layout changes considerably:

2.1 Before enqueueing:

 +--------+       +--------+       +--------+
 |#1  node|--NULL |#2  node|--NULL |#3  node|--NULL
 |        |       |        |       |        |
 | frags  |-------| frags  |-------| frags  |------NULL
 +--------+       +--------+       +--------+

This is very similar to 1.1, except that now node and frags are
different pointers, so node is just set to NULL.

2.2 After enqueueing:

 +--------+       +--------+       +--------+
 |q/slist |-------|#1  node|-------|q/slist |
 |node    |       |        |       |node    |
 |        |       | frags  |       |        |
 +--------+       +--------+       +--------+
                      |            +--------+       +--------+
                      |            |#2  node|--NULL |#3  node|--NULL
                      |            |        |       |        |
                      +------------| frags  |-------| frags  |------NULL
                                   +--------+       +--------+

When enqueuing net_buf #1, now we only enqueue that very item, instead
of enqueing the frags as well, since now node and frags are separate
pointers. This simplifies the operation and makes it atomic.

Resolves zephyrproject-rtos#52718.

Signed-off-by: Carles Cufi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants